Address PR #1925 feedback #1926
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Fix element_selector handling in template_renderer.rs - Added null check for element_selector before calling getElementById - Prevents potential JS errors with empty/null selectors 2. Remove unnecessary cargo clean from extconf.rb - Removing cached artifacts significantly increases build time - Clean CI environment doesn't need cargo clean before build 3. Extract mock request setup to helper method in form.rb - Created build_controller_with_mock_request private method - Reduces code duplication between touchpoints_js_string and render_widget_css - Improves maintainability
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses code review feedback from PR #1925 by fixing element selector handling, removing unnecessary cargo clean operations, and refactoring duplicated code in the Form model.
Key Changes:
- Added null/empty check for element_selector before JavaScript getElementById call
- Removed cargo clean command from Ruby extension build process to improve CI build performance
- Extracted mock request setup logic into a reusable private helper method
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ext/widget_renderer/src/template_renderer.rs | Adds null check for element_selector to prevent JavaScript errors with empty selectors |
| ext/widget_renderer/extconf.rb | Removes unnecessary cargo clean call to improve build performance in clean CI environments |
| app/models/form.rb | Extracts duplicated mock request setup code into a private helper method for better maintainability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Empty string '' is truthy in JavaScript when interpolated from Rust. Use length check to properly handle empty element_selector values.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses code review feedback from PR #1925:
Changes
Fix element_selector handling (
template_renderer.rs)element_selectorbefore callinggetElementByIdRemove unnecessary cargo clean (
extconf.rb)cargo cleancall before buildExtract mock request setup to helper method (
form.rb)build_controller_with_mock_requestprivate methodtouchpoints_js_stringandrender_widget_cssNot Addressed (Out of Scope)